-
Notifications
You must be signed in to change notification settings - Fork 362
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Globus] Remove the need for globus_sdk as a python dependency #337
[Globus] Remove the need for globus_sdk as a python dependency #337
Conversation
self.log.info( | ||
'Logout: Revoked tokens for user "{}" services: {}'.format( | ||
user.name, ','.join(state['tokens'].keys()) | ||
) | ||
) | ||
state['tokens'] = '' | ||
state['tokens'] = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a minor change, unrelated to removing the Globus SDK. Unit testing caught that it's possible to get tripped up by the type changing from dict to str, although in practice I think it would be difficult to get into a state where this caused an error. This change keeps types consistent, just in case.
@@ -79,7 +96,7 @@ def _authorize_url_default(self): | |||
).tag(config=True) | |||
|
|||
def _identity_provider_default(self): | |||
return os.getenv('IDENTITY_PROVIDER', 'globusid.org') | |||
return os.getenv('IDENTITY_PROVIDER', '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another change unrelated to streamlining dependencies, but I think it's preferable. The previous value of globusid.org
essentially locks down the default installation to an identity almost nobody uses. Setting this to the empty string allows the user to login with any IdP initially.
|
||
def _allow_refresh_tokens_default(self): | ||
return True | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the last refactor pulled out the code that could request refresh tokens, so this feature is no longer usable. In order to request refresh tokens, an additional parameter needs to be sent through the first leg of the OAuth flow.
for tok, v in tokens.by_resource_server.items() | ||
if tok not in self.exclude_tokens | ||
}, | ||
'tokens': by_resource_server, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole authenticate()
method is very similar to the GenericOAuthenticator
, where the only functional difference is the shape of tokens it returns. When the admin sets custom scopes, including scopes for third party resource servers secured with Globus Auth, the access/refresh tokens will come through the other_tokens
key. If the admin didn't enable auth_state
, this method behaves much the same as the GenericOAuthenticator
auth_state = await mock_globus_user.get_auth_state() | ||
assert auth_state == {'tokens': ''} | ||
assert auth_state == {'tokens': {}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Globus Test coverage is now 100%!
0435442
to
28b5dac
Compare
Rebased to include fixes for Generic tests #339. All tests pass! |
The Globus OAuthenticator currently requires a custom dependency in order to authenticate users. The Globus SDK was originally chosen for its convenience functions in generating URLs and automatically structuring tokens. However, it's not necessary and can be replaced with generic components included in this repo. With these changes, the globus_sdk can also be removed from the Zero to Jupyterhub 'hub' dependencies. While the globus_sdk is extremely useful for transferring data within the Single User Server, in the hub it is only used for the OAuthenticator.
28b5dac
to
b180ccc
Compare
Rebased on #343, which better organized the globus_sdk dependency install for Globus. These changes remove that dependency entirely to bring the Globus OAuthenticator more inline with how the other OAuthenticators work, instead of relying on oauth2 shortcuts within the globus_sdk. The implementation here is very similar to the Generic OAuthenticator, with some extensions to token handling to allow Globus Transfer tokens and any other third party tokens for user-created services. |
@NickolausDS I ran out of time for the moment to review the full PR, but I read through 50% and think everything has made sense so far. I also concluded that you are in this PR changing files in a way that will only influence globus. |
@consideRatio Correct, this only modifies Globus. #338 does affect this PR (it brings back refresh tokens), but I didn't want to add major changes outside of Thanks for reviewing! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the awesome PR and comments, and for your patience. I'm digging myself out of a PR review backlog, but slowly getting through it. Carefully commented PRs like this one are a huge help!
# Default scopes are below if unspecified. Add a custom transfer server if you have one. | ||
c.LocalGlobusOAuthenticator.scope = ['openid', 'profile', 'urn:globus:auth:scope:transfer.api.globus.org:all'] | ||
c.GlobusOAuthenticator.scope = ['openid', 'profile', 'urn:globus:auth:scope:transfer.api.globus.org:all'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -113,7 +113,6 @@ def run(self): | |||
|
|||
setup_args['extras_require'] = { | |||
'googlegroups': ['google-api-python-client==1.7.11', 'google-auth-oauthlib==0.4.1'], | |||
'globus': ['globus_sdk[jwt]>=1.0.0,<2.0.0'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
The Globus OAuthenticator currently requires a custom dependency
in order to authenticate users. The Globus SDK was originally
chosen for its convenience functions in generating URLs and
automatically structuring tokens. However, it's not necessary
and can be replaced with generic components included in this
repo.
With these changes, the globus_sdk can also be removed from the
Zero to Jupyterhub 'hub' dependencies. While the globus_sdk is
extremely useful for transferring data within the Single User
Server, in the hub it is only used for the OAuthenticator.